r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accelerator resource #7002

gazoakley · 2018-12-29T12:28:25Z

Partly addresses #6739

Changes proposed in this pull request:

New Resource: aws_globalaccelerator_accelerator

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAccAwsGlobalAcceleratorAccelerator'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./... -v -parallel 20 -run=TestAccAwsGlobalAcceleratorAccelerator -timeout 120m
?   	github.com/terraform-providers/terraform-provider-aws	[no test files]
=== RUN   TestAccAwsGlobalAcceleratorAccelerator_basic
=== PAUSE TestAccAwsGlobalAcceleratorAccelerator_basic
=== RUN   TestAccAwsGlobalAcceleratorAccelerator_update
=== PAUSE TestAccAwsGlobalAcceleratorAccelerator_update
=== RUN   TestAccAwsGlobalAcceleratorAccelerator_attributes
=== PAUSE TestAccAwsGlobalAcceleratorAccelerator_attributes
=== CONT  TestAccAwsGlobalAcceleratorAccelerator_basic
=== CONT  TestAccAwsGlobalAcceleratorAccelerator_attributes
=== CONT  TestAccAwsGlobalAcceleratorAccelerator_update
--- PASS: TestAccAwsGlobalAcceleratorAccelerator_basic (40.31s)
--- PASS: TestAccAwsGlobalAcceleratorAccelerator_update (64.26s)
--- PASS: TestAccAwsGlobalAcceleratorAccelerator_attributes (66.62s)
PASS
ok  	github.com/terraform-providers/terraform-provider-aws/aws	66.665s

…rator resource

bflad

Hey @gazoakley 👋 Thanks for submitting this! Initial feedback below. Please reach out with any questions or if you do not have time to address the items.

bflad · 2019-01-08T15:52:40Z

aws/resource_aws_globalaccelerator_accelerator.go

+			"ip_address_type": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,


Should this instead include the default for drift detection?

Suggested change

Computed: true,

Default: globalaccelerator.IpAddressTypeIpv4,

bflad · 2019-01-08T15:53:10Z

aws/resource_aws_globalaccelerator_accelerator.go

+			"enabled": {
+				Type:     schema.TypeBool,
+				Optional: true,
+				Computed: true,


Should this instead include the default for drift detection?

Suggested change

Computed: true,

Default: true,

bflad · 2019-01-08T15:54:41Z

aws/resource_aws_globalaccelerator_accelerator.go

+			"attributes": {
+				Type:     schema.TypeList,
+				Optional: true,
+				Computed: true,


The DiffSuppressFunc below should allow Computed: true to be removed here.

bflad · 2019-01-08T15:56:10Z

aws/resource_aws_globalaccelerator_accelerator.go

+						"flow_logs_enabled": {
+							Type:     schema.TypeBool,
+							Optional: true,
+							Computed: true,


Should this instead default to false for drift detection?

Suggested change

Computed: true,

Default: false,

bflad · 2019-01-08T15:56:42Z

aws/resource_aws_globalaccelerator_accelerator.go

+						"flow_logs_s3_bucket": {
+							Type:     schema.TypeString,
+							Optional: true,
+							Computed: true,


It seems like we shouldn't be ignoring this value for drift detection and should remove Computed: true.

bflad · 2019-01-08T16:13:27Z

aws/resource_aws_globalaccelerator_accelerator_test.go

+`, rInt)
+}
+
+func testAccGlobalAcceleratorAccelerator_update(rInt int) string {


This test configuration function goes away when using name instead of integer. 👍

bflad · 2019-01-08T16:15:30Z

aws/resource_aws_globalaccelerator_accelerator_test.go

+					testAccCheckGlobalAcceleratorAcceleratorExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "attributes.#", "1"),
+					resource.TestCheckResourceAttr(resourceName, "attributes.0.flow_logs_enabled", "true"),
+					resource.TestMatchResourceAttr(resourceName, "attributes.0.flow_logs_s3_bucket", regexp.MustCompile("tf-.*")),


You can use resource.TestCheckResourceAttrPair() instead of regex to ensure its value is correct.

s3BucketResourceName := "aws_s3_bucket.example" // ... resource.TestCheckResourceAttrPair(resourceName, "attributes.0.flow_logs_s3_bucket", s3BucketResourceName, "bucket"),

bflad · 2019-01-08T16:16:22Z

aws/resource_aws_globalaccelerator_accelerator_test.go

+					resource.TestMatchResourceAttr(resourceName, "name", regexp.MustCompile("tf-.*")),
+					resource.TestCheckResourceAttr(resourceName, "enabled", "false"),
+					resource.TestCheckResourceAttr(resourceName, "ip_address_type", "IPV4"),
+				),


Nit: This check should also include attributes and ip_sets in some form.

bflad · 2019-01-08T16:17:20Z

aws/resource_aws_globalaccelerator_accelerator.go

+					globalaccelerator.IpAddressTypeIpv4,
+				}, false),
+			},
+			"enabled": {


We should include an acceptance test that creates/updates enabled = true

bflad · 2019-01-08T16:28:40Z

aws/resource_aws_globalaccelerator_accelerator.go

+			State: schema.ImportStatePassthrough,
+		},
+
+		Timeouts: &schema.ResourceTimeout{


The documentation for these custom timeouts is not present on the resource documentation page, but is there a reason why they are customizable to begin with?

I thought it was good practise to offer customisable timeouts if code needs to do a wait, but I can't imagine users would want to change the defaults very often. Would it be better to add documentation or remove the ability to customize timeouts?

We generally want to err on the side of not providing it unless its dependent on some scalable resource (e.g. amount of data in a database) or fluctuates based on AWS-background processes (e.g. varying ENI removal time from load balancers when trying remove subnets). We can always add it later if necessary.

gazoakley · 2019-01-08T17:55:10Z

Happy New Year @bflad 🎉 - I'll make the updates shortly. One thing I meant to ask about was what would be the best way to deal with deletion when an accelerator is enabled - the call to DeleteAccelerator fails if enabled = true. I could:

Automatically call UpdateAccelerator to disable the accelerator, then call DeleteAccelerator
Conditionally do the above with some kind of force flag
Call DeleteAccelerator, catch the "accelerator is enabled" error and retry after disabling the accelerator

Any preference?

bflad · 2019-01-08T18:35:31Z

Interesting and happy new year as well! Got to love how none of these services are consistent. 😂 First or third option sounds good, whatever is easiest.

bflad · 2019-01-11T14:20:27Z

aws/resource_aws_globalaccelerator_accelerator_test.go

+				Config: testAccGlobalAcceleratorAccelerator_basic(rName, true),
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckGlobalAcceleratorAcceleratorExists(resourceName),
+					resource.TestCheckResourceAttr(resourceName, "name", newName),


Should be rName 😄

--- FAIL: TestAccAwsGlobalAcceleratorAccelerator_update (35.05s) testing.go:538: Step 0 error: Check failed: Check 2/3 error: aws_globalaccelerator_accelerator.example: Attribute 'name' expected "tf-acc-test-1060790186692934857", got "tf-acc-test-4121192575072923489"

Suggested change

resource.TestCheckResourceAttr(resourceName, "name", newName),

resource.TestCheckResourceAttr(resourceName, "name", rName),

Changes: * Remove `Computed: true` from `attributes` attribute * Fix `TestAccAwsGlobalAcceleratorAccelerator_update` check Output from acceptance testing: ``` --- PASS: TestAccAwsGlobalAcceleratorAccelerator_basic (55.12s) --- PASS: TestAccAwsGlobalAcceleratorAccelerator_attributes (69.74s) --- PASS: TestAccAwsGlobalAcceleratorAccelerator_update (89.72s) ```

bflad · 2019-01-15T14:00:26Z

Whoops didn't post my approval first 😅 Thanks, @gazoakley! 🚀

--- PASS: TestAccAwsGlobalAcceleratorAccelerator_basic (55.12s)
--- PASS: TestAccAwsGlobalAcceleratorAccelerator_attributes (69.74s)
--- PASS: TestAccAwsGlobalAcceleratorAccelerator_update (89.72s)

bflad · 2019-01-16T04:11:40Z

This has been released in version 1.56.0 of the AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

ghost · 2020-04-01T17:28:39Z

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

gazoakley added 2 commits December 29, 2018 11:41

r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accele…

cd78141

…rator resource

r/aws_globalaccelerator_accelerator: Initial tests

b3b0339

gazoakley added 2 commits December 29, 2018 16:57

r/aws_globalaccelerator_accelerator: Fix check for existance

275d119

Merge branch 'master' into f-globalaccelerator-accelerator

feb3b5a

ghost added size/XL Managed by automation to categorize the size of a PR. and removed size/XXL Managed by automation to categorize the size of a PR. labels Jan 3, 2019

bflad added new-resource Introduces a new resource. service/globalaccelerator Issues and PRs that pertain to the globalaccelerator service. and removed dependencies Used to indicate dependency changes. labels Jan 8, 2019

bflad reviewed Jan 8, 2019

View reviewed changes

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2019

ghost removed the waiting-response Maintainers are waiting on response from community or contributor. label Jan 8, 2019

r/aws_globalaccelerator_accelerator: Addressing review comments I

71dc8bb

bflad added the waiting-response Maintainers are waiting on response from community or contributor. label Jan 9, 2019

Merge branch 'master' into f-globalaccelerator-accelerator

dfc5950

bflad reviewed Jan 11, 2019

View reviewed changes

bflad added this to the v1.56.0 milestone Jan 15, 2019

bflad merged commit dfc5950 into hashicorp:master Jan 15, 2019

bflad added a commit that referenced this pull request Jan 15, 2019

Update CHANGELOG for #7002

e94e7e9

ghost locked and limited conversation to collaborators Apr 1, 2020

gazoakley deleted the f-globalaccelerator-accelerator branch April 7, 2020 16:17

breathingdust removed the waiting-response Maintainers are waiting on response from community or contributor. label Sep 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accelerator resource #7002

r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accelerator resource #7002

gazoakley commented Dec 29, 2018

bflad left a comment

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

bflad Jan 8, 2019

gazoakley Jan 8, 2019

bflad Jan 8, 2019

gazoakley commented Jan 8, 2019

bflad commented Jan 8, 2019

bflad Jan 11, 2019

bflad commented Jan 15, 2019

bflad commented Jan 16, 2019

ghost commented Apr 1, 2020

	Computed: true,
	Default: globalaccelerator.IpAddressTypeIpv4,

	resource.TestCheckResourceAttr(resourceName, "name", newName),
	resource.TestCheckResourceAttr(resourceName, "name", rName),

r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accelerator resource #7002

r/aws_globalaccelerator_accelerator: Add aws_globalaccelerator_accelerator resource #7002

Conversation

gazoakley commented Dec 29, 2018

bflad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gazoakley commented Jan 8, 2019

bflad commented Jan 8, 2019

Choose a reason for hiding this comment

bflad commented Jan 15, 2019

bflad commented Jan 16, 2019

ghost commented Apr 1, 2020